roborev: Combined Review (5592cb8)

Review verdict: one medium performance issue should be addressed before merge; no medium-or-higher security findings were reported.

Medium

Quadratic re-reading of Visual Studio Copilot trace files

Location: internal/parser/visualstudio_copilot.go (visualStudioCopilotSiblingTraceSpans, readVisualStudioCopilotTraceSpans) and internal/parser/discovery.go (discoverVisualStudioCopilotSessionFiles → visualStudioCopilotDiscoveryKey)

Discovery calls visualStudioCopilotDiscoveryKey for every trace file, and that helper fully reads and JSON-parses every span just to extract one gen_ai.conversation.id. Parsing a representative file then calls visualStudioCopilotSiblingTraceSpans, which reads and fully parses every other trace file in the directory to stitch siblings.

That makes a full or forced re-sync of M conversations across N files cost O(M*N) full file parses, approximately O(N^2) when most files represent distinct conversations. Since VS writes traces to an unbounded temp directory, and the data version bump forces a full re-parse, this can become expensive quickly, especially with large trace payloads.

Suggested fix: Group files by conversation ID once, reusing spans already read during discovery, or cache discovery keys by path+mtime so unchanged files are not re-parsed every sync cycle. At minimum, read only enough of each file to extract the conversation ID instead of parsing all spans.

Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (claude-code/default, done, 6m34s), codex_security (claude-code/security, done, 1m10s) | Total: 7m54s

I'm working on this

roborev: Combined Review (ca0fb73)

Summary verdict: Changes are mostly sound, but two medium parser issues can drop imported data.

Medium

• Location: internal/parser/visualstudio_copilot.go:981
  Problem: Tool-only chat outputs lose model and token usage. Executed tool-call IDs are stripped from the chat output, and the caller only appends the chat span when content or tool calls remain, so a chat span with only executed tool calls is dropped before visualStudioCopilotApplyUsage can persist gen_ai.usage.
  Fix: Preserve usage/model for suppressed tool-only chat spans, either by emitting a metadata-bearing assistant message or merging that usage onto the emitted tool execution message; add a regression test for a tool-only output with usage.

• Location: internal/parser/visualstudio_copilot.go:90
  Problem: A trace file with multiple real gen_ai.conversation.id values only emits one session. chooseVisualStudioCopilotConversation selects a single “best” conversation, and the rest are dropped, so multiple chat conversations in the same trace JSONL are never imported.
  Fix: Group spans by conversation ID and return one parse result per chat-bearing conversation, while still filtering ambient/no-prompt groups.

Panel: ci_default_security | Synthesis: codex, 17s | Members: codex_default (codex/default, done, 10m35s), codex_security (codex/security, done, 3m56s) | Total: 14m48s

roborev: Combined Review (1406c4d)

Medium issue found: Visual Studio Copilot parsing can drop conversations when multiple conversation IDs share one telemetry file.

Medium

• Location: internal/parser/visualstudio_copilot.go:90
• Problem: The parser chooses a single gen_ai.conversation.id from a trace file and returns only that session, so additional real conversations in the same Visual Studio telemetry JSONL are ignored and never imported.
• Fix: Group spans by conversation ID and emit one ParseResult per conversation, while preserving sibling-file deduplication so the same conversation is still written once.

Panel: ci_default_security | Synthesis: codex, 19s | Members: codex_default (codex/default, done, 4m20s), codex_security (codex/security, done, 1m44s) | Total: 6m23s

roborev: Combined Review (17f363f)

Summary verdict: One medium correctness issue needs attention; no security issues were reported.

Medium

• internal/parser/visualstudio_copilot.go:90
  The parser selects one “best” gen_ai.conversation.id from a trace file and returns only one session, but Visual Studio trace JSONL files can contain spans for multiple conversations. Any conversation that only appears in a file where another conversation wins this heuristic is silently never synced.

  Suggested fix: Group spans by conversation ID and emit one ParseResult per conversation, or make discovery create per-conversation work items before parsing.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 6m57s), codex_security (codex/security, done, 2m18s) | Total: 9m21s

I'm working on this to be able to merge it now

roborev: Combined Review (03b7e72)

No medium-or-higher findings; the reviewed changes look clean.

Only a low-severity symlink test portability issue was reported, so it is omitted per the requested threshold.

Panel: ci_default_security | Synthesis: codex, 4s | Members: codex_default (codex/default, done, 7m10s), codex_security (codex/security, done, 1m18s) | Total: 8m32s

rebased. will merge this once CI is green

roborev: Combined Review (6a81552)

Review verdict: one medium-risk correctness issue should be fixed before merge.

Medium

• internal/parser/visualstudio_copilot.go:290
  Sibling trace read/scan errors are silently ignored even though sessions are reconstructed from sibling files and later written with full message replacement. A transient unreadable sibling can make an existing multi-file conversation sync as a partial transcript and replace previously indexed messages.
  Fix: Return a non-cacheable parse error when sibling trace reads fail, or otherwise avoid replacing existing messages unless all sibling inputs needed for reconstruction were read successfully.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 9m11s), codex_security (codex/security, done, 3m57s) | Total: 13m14s

roborev: Combined Review (88c4eaa)

No Medium, High, or Critical findings were reported.

All reported issues were Low severity, so they are omitted per the review rules.

Panel: ci_default_security | Synthesis: codex, 5s | Members: codex_default (claude-code/default, done, 13m24s), codex_security (claude-code/security, done, 3m46s) | Total: 17m15s

roborev: Combined Review (ddc7499)

Medium issue found; no Critical or High findings.

Medium

• internal/parser/visualstudio_copilot.go:1113
  Chat spans containing only executed tool calls are discarded because executed tool calls are skipped before deciding whether to emit the assistant chat message. Those spans can still carry gen_ai.usage.* and model attributes, so tool-only LLM turns may be missing from token/model usage totals.

  Suggested fix: preserve usage from chat spans even when visible tool calls are emitted from execute_tool spans, either by attaching the chat usage to the corresponding tool message or by emitting a usage-bearing assistant message. Add a test for a pure tool_call chat span with usage attributes.

Panel: ci_default_security | Synthesis: codex, 12s | Members: codex_default (codex/default, done, 12m14s), codex_security (codex/security, done, 4m10s) | Total: 16m36s

roborev: Combined Review (071a6b0)

Summary verdict: Changes need fixes before merge; two medium-severity issues remain.

Medium

• internal/parser/visualstudio_copilot.go:435
  Chat spans with both assistant text and an executed tool call can double count token usage. visualStudioCopilotAppendChatOutput applies the chat span usage to the assistant text message, then the execute-tool branch later applies the same chat usage to the tool message because seenChatUsage is only updated there.
  Fix: Mark the chat usage identity as consumed when a chat output message receives usage, or only apply tool chat usage for chat spans that did not emit their own assistant output message.

• testdata/visualstudio-copilot/redacted/sample-4_VSGitHubCopilot_traces.jsonl:1
  The newly added “redacted” Visual Studio Copilot fixtures still contain raw get_file trace results with line-numbered source content, absolute Windows repository paths, project names, and similar raw patch/build data across the added sample files. Anyone with access to the repository or published test artifacts can read source snippets from captured local session data.
  Fix: Replace these fixtures with synthetic or fully scrubbed OTLP traces that preserve only the structural fields the parser needs. Redact file contents, absolute paths, organization/project names, patch bodies, diagnostic snippets, and stable telemetry IDs before committing, and consider adding a fixture scrubber/check.

Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 7m38s), codex_security (codex/security, done, 3m39s) | Total: 11m27s

roborev: Combined Review (f7f4a64)

Medium issue found: stale remote skip-cache entries can still suppress Visual Studio Copilot retries.

Medium

• internal/ssh/sync.go:113: The Visual Studio Copilot skip-cache migration only runs for local skipped_files in non-ephemeral engines. SSH remote sync loads remote_skipped_files after constructing an ephemeral engine and injects them unchanged, so stale remote Visual Studio Copilot read/scan-error skips from older builds can still suppress retries indefinitely when mtime is unchanged.

  Fix: Filter or migrate Visual Studio Copilot physical and virtual trace paths out of remoteCache before engine.InjectSkipCache, then persist the cleaned remote cache.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 12m59s), codex_security (codex/security, done, 2m22s) | Total: 15m29s

roborev: Combined Review (dc7f8cf)

Medium-risk issues remain; below are the deduplicated findings to include in the PR comment.

Medium

• Location: cmd/agentsview/session_export.go:65
  Problem: session export <id> resolves a Visual Studio Copilot virtual path (<traceFile>#<conversationID>) back to the physical trace file and streams the entire file. Since one trace can contain spans for multiple conversations, exporting one session can disclose unrelated conversations, including prompts, tool arguments, command output, file contents, or secrets.
  Fix: For Visual Studio Copilot virtual paths, filter JSONL output to spans whose gen_ai.conversation.id matches the requested conversation, export a normalized session representation from the database, or reject raw export for shared trace files with an explicit scoped-export alternative.

• Location: internal/parser/visualstudio_copilot.go:662
  Problem: Prompt de-duplication falls back to content:<prompt> when copilot_chat.root_request_id is absent, so two separate user turns with the same text are collapsed into one message and UserMessageCount is wrong.
  Fix: Use stable span identity for the fallback key, such as traceId/spanId, and only use content as a last resort when the span has no IDs. Add a regression test for repeated identical prompts in separate turns.

• Location: internal/sync/engine.go:4778
  Problem: Each Visual Studio Copilot conversation parse re-reads every sibling trace file, and SyncAll emits one work item per conversation. A directory with many trace files/conversations becomes conversations * files full scans, making periodic sync very expensive.
  Fix: Load and group the directory’s trace spans once per sync/physical trace path, then build all conversation sessions from that grouped data.

Panel: ci_default_security | Synthesis: codex, 11s | Members: codex_default (codex/default, done, 8m38s), codex_security (codex/security, done, 3m5s) | Total: 11m54s

roborev: Combined Review (123c853)

Summary verdict: two Medium issues should be addressed before merging.

Medium

• internal/parser/visualstudio_copilot.go:244
  Lines that fail OTLP JSON decoding are silently skipped. Malformed, partially written, or valid-but-unexpected trace lines can drop spans while the session is still written and marked current, or the file is treated as having no conversations.
  Fix: Surface decode failures as parse errors, or track malformed lines and avoid marking the session/file current when any trace line could not be decoded.

• cmd/agentsview/session_export.go:65
session export <id> resolves a Visual Studio Copilot virtual path from <traceFile>#<conversationID> to the physical trace file, then streams the whole file. Since one trace file can contain multiple conversations, exporting one session can disclose unrelated prompts, tool calls, commands, file contents, and results.
  Fix: Keep export scoped to the requested conversation ID, or refuse raw source export for shared trace files unless an explicit full-trace option is provided.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 8m23s), codex_security (codex/security, done, 2m21s) | Total: 10m52s

roborev: Combined Review (1256fe6)

Findings require changes before merge.

Medium

• internal/sync/engine.go:5911 - The Visual Studio Copilot archive-preservation guard only preserves when the reparse has fewer messages. If a rotated or deleted sibling removes tool result events or shortens content while leaving the message count unchanged, the full replacement write can overwrite a richer archived transcript and lose tool results. Compare parsed versus stored message completeness, including content length, tool call count, result event count, and token presence, similar to the existing OpenCode preservation check. Add a test for same-message-count tool result loss.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 9m37s), codex_security (codex/security, done, 3m38s) | Total: 13m21s

roborev: Combined Review (449411a)

Summary verdict: Medium correctness risks remain; no security findings were reported.

Medium

• internal/parser/visualstudio_copilot.go:244
  OTLP decode errors are silently ignored. A malformed or partially written trace line is treated as absent, so sync can force-replace a session with an incomplete transcript or cache a “no sessions” result.
  Fix: Return an error for non-empty lines that fail UnmarshalTraces, or explicitly handle only a final partial line without writing/caching partial results.

• internal/service/direct.go:270
session sync <id> resolves Visual Studio Copilot’s virtual <trace>#<conversation> path to the physical trace and sends it through SyncPaths, which reparses every conversation in that trace instead of the requested session only.
  Fix: Use SyncSingleSessionContext(ctx, in.ID) for ID-based syncs, or preserve the virtual conversation-scoped path through the direct sync path.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 9m45s), codex_security (codex/security, done, 2m59s) | Total: 12m52s

roborev: Combined Review (b56af83)

High severity finding blocks merge.

High

• internal/sync/engine.go:6071
  Visual Studio Copilot archive preservation only runs when the composite trace size shrinks, and the path also bails out when a parsed message is new or unmatched. If a rotated-away sibling contained archived messages but another trace is added or grows enough that the composite size stays the same or increases, the full replace path can drop messages whose source file no longer exists.
  Fix: Reconcile against the archived transcript based on message presence/completeness, not only currentSize < storedSize; when parsed rows include new messages, merge them into the archived transcript while retaining archived-only rows.

Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 7m48s), codex_security (codex/security, done, 3m28s) | Total: 11m23s

looking

roborev: Combined Review (bcbd8eb)

Medium finding only; security review found no additional issues.

Medium

• internal/sync/engine.go:5875 - When Visual Studio Copilot archive reconciliation chooses preserve, the write is skipped entirely, so the session’s current composite file fingerprint and data_version are never advanced. Because skip checks require matching file size/mtime and current data version, preserved sessions after trace rotation will be reparsed on every later sync instead of quiescing.
  Fix: Persist the current source fingerprint and current data version when preserving archived messages, without replacing the archived transcript. Also avoid using the old larger archive size as the skip fingerprint after merge.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 12m5s), codex_security (codex/security, done, 3m41s) | Total: 15m52s

rebasing

roborev: Combined Review (d3fe557)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 10m59s), codex_security (codex/security, done, 3m54s) | Total: 14m53s

rebasing